Skip to content

Conversation

@matts1
Copy link
Contributor

@matts1 matts1 commented Oct 21, 2025

We will support arbitrary hooks in the future, but for a few reasons we will special-case jj fix:

  • Performance (jj fix is hyper-optimized for running on stacks of commits at the same time)
  • Common (This will probably be the most common requested pre-upload check)
  • Simplicity (this was much easier to implement, so is a good first hook)

Bug: #3577

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added/updated tests to cover my changes

@matts1 matts1 force-pushed the push-qsozopkzlzrn branch 2 times, most recently from 2d9fb18 to 5e574e4 Compare October 21, 2025 07:04
@matts1 matts1 marked this pull request as ready for review October 21, 2025 07:04
@matts1 matts1 requested a review from a team as a code owner October 21, 2025 07:04
@matts1 matts1 force-pushed the push-qsozopkzlzrn branch from 5e574e4 to 38f4ab3 Compare October 21, 2025 07:09
Copy link
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor stuff, I also don't really agree with how the code currently was rewritten but I'm unable to point at it.

cc @hooper

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: In the commit description you could mention that "you" (i.e the chromium project) scripted around that which is imo a good motivation.

Comment on lines +28 to +94
/// Currently, this triggers on `jj gerrit upload`. Other forges which
/// implement custom upload scripts should also call this.
///
/// This should ideally work for `jj git push` too, but doing so has
/// consequences. `git push` can be used to upload to code review, but it can
/// do many other things as well. We need to ensure the UX works well before
/// adding it to `git push`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should advertise this as "general hook" support. Since it is the only support we'll ever have

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intend to add support for general hooks in the future. This is why I'm working on repo-managed config. Having that will allow us to do this in a secure manner.

@thoughtpolice
Copy link
Member

We already have some "on push" behavior, namely signing. I had an old diff I never upstreamed that added a new flag called on-push-behavior = ["..."] to slightly generalize this; the cropped diff looks like:

diff --git a/cli/src/commands/git/push.rs b/cli/src/commands/git/push.rs
index 88403257ea..fee554e83b 100644
--- a/cli/src/commands/git/push.rs
+++ b/cli/src/commands/git/push.rs
@@ -393,7 +393,7 @@
         return Ok(());
     }

-    let sign_behavior = if tx.settings().get_bool("git.sign-on-push")? {
+    let sign_behavior = if should_sign_on_push(tx.settings())? {
         Some(SignBehavior::Own)
     } else {
         None
@@ -1016,3 +1016,15 @@
         .collect_vec();
     Ok(bookmarks_targeted)
 }
+
+fn should_sign_on_push(settings: &UserSettings) -> Result<bool, CommandError> {
+    // First check the new on-push-behavior configuration
+    if let Ok(behaviors) = settings.get::<Vec<String>>("git.on-push-behavior") {
+        return Ok(behaviors.contains(&"sign".to_string()));
+    } else if let Some(behavior) = settings.get_string("git.on-push-behavior").optional()? {
+        return Ok(behavior == "sign");
+    }
+
+    // Fall back to the old sign-on-push configuration for backward compatibility
+    settings.get_bool("git.sign-on-push").map_err(Into::into)
+}
diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json
index e4ed1a4c81..da11add710 100644
--- a/cli/src/config-schema.json
+++ b/cli/src/config-schema.json
@@ -468,6 +468,22 @@
                     "description": "Whether jj should sign commits before pushing",
                     "default": false
                 },
+                "on-push-behavior": {
+                    "description": "List of behaviors to execute before pushing",
+                    "oneOf": [
+                        {
+                            "type": "string",
+                            "enum": ["sign"]
+                        },
+                        {
+                            "type": "array",
+                            "items": {
+                                "type": "string",
+                                "enum": ["sign"]
+                            }
+                        }
+                    ]
+                },
                 "track-default-bookmark-on-clone": {
                     "type": "boolean",
                     "description": "Whether `jj git clone` creates a local bookmark tracking the default remote bookmark",

I had then wanted to extend jj fix to also use this flag, instead. I think that would be a nicer configuration, all things considered. We could even make the entry a table and allow you to specify different behaviors depending on branch patterns, I suppose... WDYT?

FWIW, I'm not so sure about special casing it only on gerrit upload, that comes across as a bit strange. When would it be bad to run jj fix before a push? I don't think anyone has ever reported any issues about this coming up with the signing behavior (please correct me if I'm wrong), so I'm not sure that special casing it is really necessary, tbh.

@PhilipMetzger
Copy link
Contributor

I had then wanted to extend jj fix to also use this flag, instead. I think that would be a nicer configuration, all things considered. We could even make the entry a table and allow you to specify different behaviors depending on branch patterns, I suppose... WDYT?

I agree but ideally we really shouldn't run untrusted programs but should use user-aliases instead. This will let users plug a bunch of with the model. And matching on bookmark patterns also makes sense.

@matts1
Copy link
Contributor Author

matts1 commented Oct 24, 2025

We already have some "on push" behavior, namely signing. I had an old diff I never upstreamed that added a new flag called on-push-behavior = ["..."] to slightly generalize this; the cropped diff looks like:

I have a follow-up PR that adds support for generalized hooks rather than just supporting fix. I don't like having it as a list though, since that introduces several UX issues:

  • To disable one specific hook is difficult
    • You need to set --config=on-push-behaviour = ["foo", "bar"] and know what the names of these behaviours are
  • To add on to existing hooks is difficult
    • I set the config in jj config edit --user to ["fix", "lint"]
    • The repo sets the default to ["fix"]
    • The repo has now overridden my config

I believe that an approach such as what jj fix already has for their tools is a better approach.

My intent was to have specifically hooks.pre-upload-fix and hooks.pre-upload.foo for arbitrary tools. Maybe hooks.pre-upload.fix would be a better idea for consistency. I wanted to collect them under hooks because I intended to add more different types of hooks.

I agree but ideally we really shouldn't run untrusted programs but should use user-aliases instead. This will let users plug a bunch of with the model. And matching on bookmark patterns also makes sense.

This is one of the reasons I'm working on secure configuration and secure repo configuration. I intend to do something similar to fix tools in the future, which should make this secure.

We will support arbitrary hooks in the future, but for a few reasons we will special-case `jj fix`:
* Performance (jj fix is hyper-optimized for running on stacks of
  commits at the same time)
* Common (This will probably be the most common requested pre-upload check)
* Simplicity (this was much easier to implement, so is a good first
  hook)

Bug: jj-vcs#3577
@matts1 matts1 force-pushed the push-qsozopkzlzrn branch from 38f4ab3 to 9fc5a68 Compare October 24, 2025 02:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants